feat: Clean up StateDB methods, bump revm#238
feat: Clean up StateDB methods, bump revm#2380xForerunner wants to merge 3 commits intoalloy-rs:mainfrom
Conversation
0xForerunner
force-pushed
the
state-db-methods
branch
from
bb14179 to
0833666
Compare
|
iiuc those changes are needed just to make |
|
@klkvr We need the extra trait methods for this PR into reth. The ultimate reason is for block access lists. We have a BAL implementation which requires the use of a wrapper type around State. This isn't possible without these methods. |
0xForerunner
force-pushed
the
state-db-methods
branch
from
f2b555d to
3e5e8f3
Compare
|
@klkvr just rebased on main. Should be good to go! |
|
@0xForerunner it's still not obvious to me why we need those methods to be honest. does your BAL implementation require the block builder to hold your custom database? |
|
@klkvr You can see the implementation of StateDB for our BalBuilderDb here When we're executing the bal we need access to the bundle state, like here And we need merge_transitions here You can see our BalBuilderDb wraps the underlying DB Lemme know if you need anything else to justify these changes! |
0xForerunner
force-pushed
the
state-db-methods
branch
from
6408e10 to
5baff2f
Compare
|
@klkvr rebased. Should be ready again. |
|
Hey @mattsse If you mind taking a look at this it would be swell! Maintaining this fork though alloy-rs/evm -> reth -> op-reth is a bit painful :p |
| /// we at any time revert state of bundle to the state before transition | ||
| /// is applied. | ||
| fn merge_transitions(&mut self, retention: BundleRetention); | ||
| } |
There was a problem hiding this comment.
it is not entirely clear to me why this abstraction is needed, what is your usecase? right now basic block executors are already agnostic of concrete db
There was a problem hiding this comment.
We need this abstraction because our access list block builder needs to be able to wrap revm::State
There was a problem hiding this comment.
ah yep sorry missed it, just to confirm — you need this so that your custom block builder can use BasicBlockBuilder::finish API directly instead of duplicating the logic?
i'm quite happy with the way current abstraction works in a sense that it allows any database to be used for a BlockExecutor. if we need an additional abstraction for State db specific methods i'd prefer it to not affect BlockExecutorFactory trait at all but instead only be used for the reth's block builder construction methods
i'd be open to moving StateDB trait entirely into reth for this
There was a problem hiding this comment.
I'm not entirely sure what you're suggesting. We already have StateDB used in the BlockExecutor impls. What is your reasoning for not wanting this? The previous implementation relied directly on revm::State, which is strictly less flexible.
There was a problem hiding this comment.
yep and current implementation works with any database which is very flexible which is why I'm a bit hesitant on making it stricter
2 participants
This PR removes unnecessary methods from
StateDBin favour of usingDatabaseCommitExt. See #234 and this pr for more details.Additionally we've added a few more trait methods to abstract away from
revm::State.You can see why this is required by taking a look at this pr into reth.
PR Checklist